Codecov Report

❗ No coverage uploaded for pull request base (branch-23.04@e4557cb). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head f0ae3e2 differs from pull request most recent head 855fcc0. Consider uploading reports for the commit 855fcc0 to get more accurate results

@@               Coverage Diff               @@
##             branch-23.04   #12637   +/-   ##
===============================================
  Coverage                ?   43.53%           
===============================================
  Files                   ?      158           
  Lines                   ?    24868           
  Branches                ?        0           
===============================================
  Hits                    ?    10827           
  Misses                  ?    14041           
  Partials                ?        0           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

FYI, we need to fix some issues with the way rmm is packaging/using spdlog. I have a PR here that moves in that direction, but more changes may be needed. Please don’t merge this PR until I can confirm that rmm is fixed up. See: rapidsai/rmm#1177

You may want to make the design a bit more flexible such that it doesn't assume one is always logging to a file.

@jlowe or @revans2 can correct me if I'm wrong, but iirc Spark wants to be able to log to something like cout/cerr. In the RMM logging adaptor we enabled this by adding a constructor that takes an arbitrary std::ostream: https://github.com/rapidsai/rmm/blob/58ed5c9c373e8859bb009b32cd1f7dd0522e1a7d/include/rmm/mr/device/logging_resource_adaptor.hpp#L78-L98

Spark wants to be able to log to something like cout/cerr

We'd like the ability to glue the cudf logging into the existing Spark/Java logging mechanisms, e.g.: log4j. Being able to specify a custom spdlog sink, ideally at runtime, would give us the flexibility to do that and more.

The singleton spdlog object is accessible via cudf::logger() (unless I broke something when moving definitions to .cpp). It can be used to fully configure logging, including a custom sink.

One caveat: the API that changes the sink is not thread-safe. @jlowe is this an issue?
Here's a sample of how logger can be modified at runtime (will add samples to PR through tests):

std::ostringstream oss;
// Divert the log to the string stream (not thread-safe!)
cudf::logger().sinks() = {std::make_shared<spdlog::sinks::ostream_sink_mt>(oss)};
// Only include the log message (no timestamp, etc.)
cudf::logger().set_formatter(
  std::unique_ptr<spdlog::formatter>(new spdlog::pattern_formatter("%v")));
// Only log warnings and worse
cudf::logger().set_level(spdlog::level::warn);

The fmt/spdlog conda packaging looks good. The fmt/spdlog CMake packaging looks good to me also, but I would like a second eye from @robertmaynard before merging.

Looks good to me

We should see if the numbers in rapidsai/rmm#1222 also accurately capture what happens to compile times here.

Has anything happened with this? I'm curious how urgent it is to change RMM's spdlog dependency to use the pre-compiled library.

We should see if the numbers in rapidsai/rmm#1222 also accurately capture what happens to compile times here.

Has anything happened with this? I'm curious how urgent it is to change RMM's spdlog dependency to use the pre-compiled library.

I tried using the build metrics to evaluate the impact of spdlog dependency. There's a measurable impact on .so size, but compilation time seems hard to compare because of caching. The time difference does not look alarming given the difference between cache hit rates.
no logging

with logging

/merge